Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bug: propagate TLS errors as failures #73

Merged
merged 10 commits into from
Apr 24, 2024
Merged

bug: propagate TLS errors as failures #73

merged 10 commits into from
Apr 24, 2024

Conversation

ainghazal
Copy link
Collaborator

@ainghazal ainghazal commented Apr 2, 2024 •
edited
Loading

Description

For integration into probe-cli, we need to propagate any errors raised during TLS handshake.

While working on this commit, a few other improvements were made:

  • the minivpn exported tracer accepts a transaction ID, for compatibility with the array semantics used in the probe engine.
  • minivpn was adapted to work with a downgraded version of a few dependencies, to keep probe-cli dependency on go1.20.x:
    • gateways (used for getting the default gateway for routes when using a system tun device) is pinned to a go1.20-compatible version
    • package slices is changed in favor of 1.20 compat

Checklist

  • I have read the contribution guidelines
  • Iff you changed code related to services, or inter-service communication, make sure you update the diagrams in ARCHITECTURE.md.
  • Reference issue for this pull request:

@ainghazal
Copy link
Collaborator Author

Do note that this PR depends on #72, I will rebase it after #72 is merged.

@ainghazal ainghazal requested a review from hellais April 2, 2024 18:24
@ainghazal ainghazal self-assigned this Apr 2, 2024
@ainghazal ainghazal added the bug Something isn't working label Apr 2, 2024
@ainghazal
Copy link
Collaborator Author

@hellais rebased, ready to review

hellais
hellais reviewed Apr 24, 2024
View reviewed changes
internal/tlssession/tlssession.go Outdated
// TODO(ainghazal): pass the failure to the tracer too.

if errors.Is(err, ErrBadCA) {
ws.sessionManager.Failed <- session.NewFailure(err)
Copy link
Member

@hellais hellais Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you need to wrap this in a special Failure struct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention was to be able to extend it in the future for different types of failures, but you're right YAGNI for now; I'll refactor to just pass the error for now.

hellais
hellais approved these changes Apr 24, 2024
View reviewed changes
Copy link
Member

@hellais hellais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked a clarifying question about the need to wrap stuff in a custom Failure struct, but otherwise LGTM!

hellais
hellais approved these changes Apr 24, 2024
View reviewed changes
Copy link
Member

@hellais hellais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐳

@ainghazal ainghazal merged commit 562e51c into ooni:main Apr 24, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hellais hellais hellais approved these changes

Assignees

@ainghazal ainghazal

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ainghazal @hellais